Skip to content

[Dynamic Dashboard] Improvements to the custom range selection logic #11510

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
May 14, 2024
Merged

[Dynamic Dashboard] Improvements to the custom range selection logic #11510

merged 3 commits into from
May 14, 2024

Conversation

hichamboushaba
Copy link
Member

@hichamboushaba hichamboushaba commented May 14, 2024 •
edited
Loading

Closes: #11509

Description

This PR fixes the logic of the custom range in the top performers card, the logic is as follows now:
When selecting the custom ranges dropdown entry, if a custom range has already been saved, it will be used directly; otherwise, the date picker will be shown.

The PR also fixes an issue where dismissing the date picker would not cancel the change to the "Custom Range" entry.

Testing instructions

  1. Clear the app's data for the following steps (you can alternatively remove the datastore files from the storage).
  2. Open the app.
  3. In the performance card, tap on the range button.
  4. Select "Custom Range"
  5. Confirm the date picker is shown.
  6. Dismiss it.
  7. Confirm the range didn't change.
  8. Re-select "Custom Range"
  9. Choose a range in the date picker.
  10. Confirm the range was updated.
  11. Switch to a different range.
  12. Switch to the "Custom range"
  13. Confirm the previous range is applied automatically.
  14. Repeat 3 to 13 for the top performers card.

Images/gif

  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

@hichamboushaba hichamboushaba added type: bug A confirmed bug. feature: dashboard Related to home screen project labels May 14, 2024
@wpmobilebot
Copy link
Collaborator

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
FlavorJalapeno
Build TypeDebug
Commitec699ee
Direct Downloadwoocommerce-prototype-build-pr11510-ec699ee.apk

hichamboushaba
hichamboushaba commented May 14, 2024
View reviewed changes
WooCommerce/src/main/kotlin/com/woocommerce/android/AppPrefs.kt
@@ -861,11 +861,11 @@ object AppPrefs {

fun getActiveStatsTab() = getString(DeletablePrefKey.ACTIVE_STATS_GRANULARITY)

fun setActiveTopPerformersGranularity(selectionName: String) {
fun setActiveTopPerformersTab(selectionName: String) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This renaming was done just for consistency, the term granularity doesn't apply to tabs anymore after introducing the custom range.

hichamboushaba
hichamboushaba commented May 14, 2024
View reviewed changes
...kotlin/com/woocommerce/android/ui/dashboard/topperformers/DashboardTopPerformersViewModel.kt
@@ -102,8 +107,6 @@ class DashboardTopPerformersViewModel @AssistedInject constructor(
)
}.asLiveData()

private val customRange = customDateRangeDataStore.dateRange.asLiveData()
Copy link
Member Author

@hichamboushaba hichamboushaba May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LiveData here wasn't observed anywhere, which means it always had the value as null, and it caused a bug where opening the date picker doesn't show the current custom range.

@hichamboushaba hichamboushaba requested a review from 0nko May 14, 2024 19:05
@hichamboushaba hichamboushaba added this to the 18.7 milestone May 14, 2024
@hichamboushaba hichamboushaba added the status: feature-flagged Behind a feature flag. Milestone is not strongly held. label May 14, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 2.32558% with 42 lines in your changes are missing coverage. Please review.

Project coverage is 40.82%. Comparing base (5ed1a5c) to head (ec699ee).
Report is 2 commits behind head on trunk.

Files Patch % Lines
...d/topperformers/DashboardTopPerformersViewModel.kt 0.00% 23 Missing ⚠️
.../topperformers/DashboardTopPerformersWidgetCard.kt 0.00% 11 Missing ⚠️
...roid/ui/dashboard/stats/DashboardStatsViewModel.kt 20.00% 3 Missing and 1 partial ⚠️
.../kotlin/com/woocommerce/android/AppPrefsWrapper.kt 0.00% 2 Missing ⚠️
...rc/main/kotlin/com/woocommerce/android/AppPrefs.kt 0.00% 1 Missing ⚠️
...ashboard/stats/GetSelectedRangeForTopPerformers.kt 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##              trunk   #11510      +/-   ##
============================================
- Coverage     40.83%   40.82%   -0.02%     
  Complexity     5180     5180              
============================================
  Files          1070     1070              
  Lines         62310    62327      +17     
  Branches       8499     8502       +3     
============================================
  Hits          25444    25444              
- Misses        34578    34595      +17     
  Partials       2288     2288

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hichamboushaba hichamboushaba marked this pull request as ready for review May 14, 2024 20:18
@0nko 0nko self-assigned this May 14, 2024
0nko
0nko approved these changes May 14, 2024
View reviewed changes
Copy link
Contributor

@0nko 0nko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good 👍

@0nko 0nko merged commit 7ab8918 into trunk May 14, 2024
22 checks passed
@0nko 0nko deleted the issue/11509-top-performers-date-picker branch May 14, 2024 22:30
@sentry-io Sentry.io
Copy link

sentry-io bot commented May 15, 2024 •
edited
Loading

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Resources$NotFoundException: String resource ID #0x0 Dashboard View Issue
  • ‼️ IndexOutOfBoundsException: Index 2 out of bounds for length 2 com.woocommerce.android.util.DateUtils in getLo... View Issue
  • ‼️ IndexOutOfBoundsException: Index 2 out of bounds for length 2 com.woocommerce.android.util.DateUtils in getSh... View Issue
  • ‼️ ArrayIndexOutOfBoundsException: length=12; index=19 Dashboard View Issue
  • ‼️ NumberFormatException: For input string: "20 14" com.woocommerce.android.util.DateUtils in getLo... View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0nko 0nko 0nko approved these changes

Assignees

@0nko 0nko

Labels
feature: dashboard Related to home screen project status: feature-flagged Behind a feature flag. Milestone is not strongly held. type: bug A confirmed bug.
Projects
None yet
Milestone
18.7
Development

Successfully merging this pull request may close these issues.

[Dynamic Dashboard] Top Performers card doesn't show date picker when adding custom range for the first time
4 participants
@hichamboushaba @wpmobilebot @codecov-commenter @0nko